-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[bugfix] add missing endpoints to admin API interface #316
base: main
Are you sure you want to change the base?
Conversation
I don't think any of these should be exposed. There all about key/app updates and don't function in useful ways. Maybe update coordinators could be exposed but I'd prefer the first two weren't |
I agree on principle; the issue is that they're already exposed in the concrete implementation |
Those calls are experimental, but I think it's okay to expose them, because we didn't retract the countersigning calls either, for example. What I don't understand is in which way is this blocking you, @pdaoust? You're saying you're linking from JS to Rust docs. If these methods don't exist on the JS API, then there's nothing to link from. What is the problem there? |
@jost-s they do exist on the |
@pdaoust Ah, okay. I suggest not linking from the payloads to API calls. Some payloads are used in multiple places, and an API doc page like this gives an excellent overview over which call uses which payload https://github.com/holochain/holochain-client-js/blob/main/docs/client.adminwebsocket.md It'll be even easier to see for each call once we refactor the JS client to use less indirections like the |
hm, maybe this is another case of me thinking about things too rigorously and not considering that people's mode of discovering these input/output payloads will 90% be via the interface's list of methods. Glad to hear the It does bother me that the But anyhow @jost-s regarding the subject of the PR, does it look okay and merge-worthy? Feels like a "why not; it's just a few lines" |
As it's not blocking you, @pdaoust, I'd follow @ThetaSinner's request to only expose the update coordinators call. |
combing through documentation and discovered that there are methods on
AdminWebsocket
that aren't defined in theAdminApi
interface.